Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

airsonic: fix virtualHost option #78129

Merged
merged 1 commit into from
Apr 15, 2020
Merged

Conversation

flyfloh
Copy link
Contributor

@flyfloh flyfloh commented Jan 20, 2020

The webplayer was not working with the services.airsonic.virtualHost
option because of missing proxy headers. This sets these headers up as
described in the airsonic documentation.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Jan 20, 2020
@flyfloh flyfloh requested a review from aanderse January 25, 2020 11:24
@aanderse
Copy link
Member

aanderse commented Jan 25, 2020

@flyfloh have you already tried setting services.nginx.recommendedProxySettings = true;?

@flyfloh
Copy link
Contributor Author

flyfloh commented Feb 4, 2020

@aanderse Sorry, took a while until I found some time to try this. Unfortunately setting recommendedProxySettings breaks when there are other virtual hosts configured on the same machine and renders airsonic completely inaccessible.

@aanderse
Copy link
Member

aanderse commented Feb 6, 2020

No problem. This looks fine to me then. We should find someone familiar with nginx to approve this and then we can merge 👍

@aanderse aanderse requested review from rnhmjoj, Ma27 and danbst February 16, 2020 22:03
@rnhmjoj
Copy link
Contributor

rnhmjoj commented Feb 16, 2020

@aanderse Sorry, took a while until I found some time to try this. Unfortunately setting recommendedProxySettings breaks when there are other virtual hosts configured on the same machine and renders airsonic completely inaccessible.

It seems all right to use their settings, though I wonder what's going on here: what if other services are affected as well?
A diff between ours recommendedProxySettings and airsonic recommendation shows that:

  1. we set an extra Accept-Encoding "", probably unrelated
  2. they set proxy_max_temp_file_size 0, probably unrelated
  3. we set an extra X-Forwarded-Server $host, not sure but probably harmless
  4. finally, we disagree on the value of X-Forwarded-Host: we use $host while they use $http_host.

The nginx docs says the following about $host

in this order of precedence: host name from the request line, or host name from the “Host” request header field, or the server name matching a request

On the other hand $http_host:

The value of the HTTP request header HOST when converted to lowercase and with 'dashes' converted to 'underscores'

The two could be different and thus break the connection if

  • The server is not on port 80 or 443, or
  • The virtual host has several names

@aanderse
Copy link
Member

ping @flyfloh

@flyfloh
Copy link
Contributor Author

flyfloh commented Feb 28, 2020

I couldn't reproduce the breakage when using recommendedProxySettings when trying to investigate this further just now. Since everything works with that setting now I have updated this PR to use it.

@flyfloh flyfloh requested a review from Ma27 February 28, 2020 18:36
This fixes music playback when using the `services.airsonic.virtualHost`
option.
@rnhmjoj
Copy link
Contributor

rnhmjoj commented Apr 13, 2020

So, can we go forward with this? Should it be backported?

@flyfloh
Copy link
Contributor Author

flyfloh commented Apr 14, 2020

Everything still works from my side. Is there anything I need to do before this can be merged?

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Apr 15, 2020

Since @Ma27 approved of this and you can't reproduce the issue with recommendedProxySettings I think it's good to go.

@rnhmjoj rnhmjoj merged commit da232ea into NixOS:master Apr 15, 2020
@rnhmjoj
Copy link
Contributor

rnhmjoj commented Apr 15, 2020

Backported to 20.03 in e393449.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants